Batch tpu calls in send-transaction-service #24083

lijunwangs · 2022-04-02T23:50:40Z

Problem

Send transaction in batches to improve throughput

Summary of Changes

Introduced flag --tpu-do-batch2.
Introduced flag to control the batch size-- by default 100
The default batch timeout is 200ms -- configurable. If either it time out or the batch size is filled, a new batch is sent
The batch honor the retry rate on the transaction already sent before.
Introduced two threads in STS: one for receiving new transactions and doing batch send and one for retrying old transactions and doing batch.6.
Fixes #

codecov · 2022-04-04T23:13:44Z

Codecov Report

Merging #24083 (697ea14) into master (255a6a7) will decrease coverage by 0.0%.
The diff coverage is 89.5%.

❗ Current head 697ea14 differs from pull request most recent head aca582a. Consider uploading reports for the commit aca582a to get more accurate results

@@            Coverage Diff            @@
##           master   #24083     +/-   ##
=========================================
- Coverage    82.0%    81.9%   -0.1%     
=========================================
  Files         593      593             
  Lines      163827   163956    +129     
=========================================
+ Hits       134340   134399     +59     
- Misses      29487    29557     +70

t-nelson

I think this change set can be much cleaner if we don't special-case batching and instead just batch everything with default batch size of one. Thoughts @CriesofCarrots ?

validator/src/main.rs

send-transaction-service/src/send_transaction_service.rs

lijunwangs · 2022-04-05T16:31:31Z

I think this change set can be much cleaner if we don't special-case batching and instead just batch everything with default batch size of one. Thoughts @CriesofCarrots ?

The logics specially added for batching has to be there -- unless you are questioning the logic itself. Using batch size = 1 which is actually disabling batching does not change that fact.

t-nelson · 2022-04-05T16:47:58Z

I think this change set can be much cleaner if we don't special-case batching and instead just batch everything with default batch size of one. Thoughts @CriesofCarrots ?

The logics specially added for batching has to be there -- unless you are questioning the logic itself. Using batch size = 1 which is actually disabling batching does not change that fact.

That's exactly what I'm suggesting. All RPC STS sends should use batching, the default is just size = 1. That is, we drop the old logic entirely in favor of the new batching logic. It will be much cleaner and less bug-prone than this shoehorn implementation

lijunwangs · 2022-04-05T16:52:54Z

I think this change set can be much cleaner if we don't special-case batching and instead just batch everything with default batch size of one. Thoughts @CriesofCarrots ?

The logics specially added for batching has to be there -- unless you are questioning the logic itself. Using batch size = 1 which is actually disabling batching does not change that fact.

That's exactly what I'm suggesting. All RPC STS sends should use batching, the default is just size = 1. That is, we drop the old logic entirely in favor of the new batching logic. It will be much cleaner and less bug-prone than this shoehorn implementation

I don't think it makes sense to drop the old logic all together in case batch size = 1 -- for example firing right away without waiting for further entries. In my opinion, keep that logic is important till we get the confidence of the batch change.

lijunwangs · 2022-04-05T17:50:26Z

I think this change set can be much cleaner if we don't special-case batching and instead just batch everything with default batch size of one. Thoughts @CriesofCarrots ?

The logics specially added for batching has to be there -- unless you are questioning the logic itself. Using batch size = 1 which is actually disabling batching does not change that fact.

That's exactly what I'm suggesting. All RPC STS sends should use batching, the default is just size = 1. That is, we drop the old logic entirely in favor of the new batching logic. It will be much cleaner and less bug-prone than this shoehorn implementation

I don't think it makes sense to drop the old logic all together in case batch size = 1 -- for example firing right away without waiting for further entries. In my opinion, keep that logic is important till we get the confidence of the batch change.

Maybe we can get rid of the send_transaction being called multiple places.

multinode-demo/validator.sh

multinode-demo/bootstrap-validator.sh

validator/src/main.rs

send-transaction-service/src/send_transaction_service.rs

validator/src/main.rs

send-transaction-service/src/send_transaction_service.rs

validator/src/main.rs

send-transaction-service/src/send_transaction_service.rs

CriesofCarrots · 2022-04-12T04:29:15Z

Fyi, I should have a bench-tps that targets Rpc/STS tomorrow-ish. I have tested rates yet, but hopefully it will be enough TPS to test the pathways you want here.
And if my review is still desired, I'll then look at this.

lijunwangs · 2022-04-12T05:37:29Z

Fyi, I should have a bench-tps that targets Rpc/STS tomorrow-ish. I have tested rates yet, but hopefully it will be enough TPS to test the pathways you want here.
And if my review is still desired, I'll then look at this.

Thanks @CriesofCarrots. It would be great if you can also examine this logic as the change is relatively large in a high traffic path.

send-transaction-service/src/send_transaction_service.rs

t-nelson · 2022-04-14T07:11:47Z

let me know when this is ready for another pass

lijunwangs · 2022-04-14T08:28:21Z

let me know when this is ready for another pass

Thanks Trent -- the code itself can be reviewed -- it handle the case we are doing async in none batch mode. Right now I am working on setting up a cluster to validate if the batching is making a difference. Will post some results.

lijunwangs · 2022-04-15T19:05:01Z

Investigating the local-cluster test failure -- seems to be related to the changes in this PR...

lijunwangs · 2022-04-20T22:38:19Z

r+ @CriesofCarrots approval

Hi Tyera -- will you be able to give it one more pass? Thanks @t-nelson for all the great feedback!

CriesofCarrots · 2022-04-20T22:48:00Z

Yep, I'm about halfway through. I have to step afk for a bit, but it will be done tonight!

CriesofCarrots

Okay, I think all I have are nits!
I saw a few of the things I see you and Trent already talked about, so looking forward to the iterations.

However, one question just to make sure I understand: the batch_send_rate_ms only affects the initial send, right? After that, batches are sent on the retry_rate_ms schedule?

CriesofCarrots · 2022-04-20T22:02:20Z

multinode-demo/bootstrap-validator.sh

@@ -61,6 +61,15 @@ while [[ -n $1 ]]; do
    elif [[ $1 = --enable-rpc-bigtable-ledger-storage ]]; then
      args+=("$1")
      shift
+    elif [[ $1 = --tpu-use-quic ]]; then


Thanks for plumbing these

Correct @CriesofCarrots. This is a heuristic controls to avoid extreme configurations of the two parameters. It is not a full rate limiting implementation. That will require another PR to address.

validator/src/main.rs

send-transaction-service/src/send_transaction_service.rs

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

Pull request has been modified.

CriesofCarrots

lgtm!

Introduced flag --tpu-do-batch2. Introduced flag to control the batch size-- by default 100 The default batch timeout is 200ms -- configurable. If either it time out or the batch size is filled, a new batch is sent The batch honor the retry rate on the transaction already sent before. Introduced two threads in STS: one for receiving new transactions and doing batch send and one for retrying old transactions and doing batch.6. Fixes # (cherry picked from commit 7c61e43) # Conflicts: # validator/src/main.rs

Introduced flag --tpu-do-batch2. Introduced flag to control the batch size-- by default 100 The default batch timeout is 200ms -- configurable. If either it time out or the batch size is filled, a new batch is sent The batch honor the retry rate on the transaction already sent before. Introduced two threads in STS: one for receiving new transactions and doing batch send and one for retrying old transactions and doing batch.6. Fixes #

lijunwangs force-pushed the batch_tpu_calls branch 2 times, most recently from 6fc5fd1 to 2f9d23b Compare April 4, 2022 18:50

lijunwangs requested review from t-nelson, CriesofCarrots and sakridge April 4, 2022 22:47

t-nelson reviewed Apr 5, 2022

View reviewed changes

lijunwangs commented Apr 6, 2022

View reviewed changes

multinode-demo/validator.sh Outdated Show resolved Hide resolved

lijunwangs commented Apr 6, 2022

View reviewed changes

multinode-demo/bootstrap-validator.sh Outdated Show resolved Hide resolved

validator/src/main.rs Outdated Show resolved Hide resolved

validator/src/main.rs Outdated Show resolved Hide resolved

t-nelson reviewed Apr 6, 2022

View reviewed changes

t-nelson reviewed Apr 7, 2022

View reviewed changes

ryleung-solana reviewed Apr 7, 2022

View reviewed changes

send-transaction-service/src/send_transaction_service.rs Outdated Show resolved Hide resolved

lijunwangs force-pushed the batch_tpu_calls branch 2 times, most recently from 5d03a97 to c05429c Compare April 9, 2022 17:50

t-nelson reviewed Apr 12, 2022

View reviewed changes

lijunwangs force-pushed the batch_tpu_calls branch from 58a0cf4 to a8760a6 Compare April 12, 2022 19:47

lijunwangs commented Apr 12, 2022

View reviewed changes

send-transaction-service/src/send_transaction_service.rs Outdated Show resolved Hide resolved

lijunwangs force-pushed the batch_tpu_calls branch 2 times, most recently from 03bc1bd to d3932a2 Compare April 15, 2022 05:34

lijunwangs force-pushed the batch_tpu_calls branch from b6ca109 to d9f4a29 Compare April 18, 2022 16:35

CriesofCarrots reviewed Apr 21, 2022

View reviewed changes

lijunwangs and others added 6 commits April 21, 2022 09:27

Addressed some feedback from Tyera

e439890

Update send-transaction-service/src/send_transaction_service.rs

9007a6e

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

Update send-transaction-service/src/send_transaction_service.rs

78f5285

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

Update send-transaction-service/src/send_transaction_service.rs

afc760d

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

Update send-transaction-service/src/send_transaction_service.rs

c8b4e9a

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

Update send-transaction-service/src/send_transaction_service.rs

023bd7b

Co-authored-by: Tyera Eulberg <teulberg@gmail.com>

lijunwangs added 2 commits April 21, 2022 09:31

Addressed more review feedback from Tyera

2a88c62

Merge branch 'leader_info_refresher' into batch_tpu_calls

aca582a

CriesofCarrots approved these changes Apr 21, 2022

View reviewed changes

lijunwangs merged commit 7c61e43 into solana-labs:master Apr 21, 2022

lijunwangs added the v1.10 label Apr 21, 2022

mergify bot mentioned this pull request Apr 21, 2022

Batch tpu calls in send-transaction-service (backport #24083) #24562

Closed

ryleung-solana mentioned this pull request Jun 20, 2022

Update RPC-STS and Transaction Forwarding to use #22916

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch tpu calls in send-transaction-service #24083

Batch tpu calls in send-transaction-service #24083

lijunwangs commented Apr 2, 2022 •

edited

Loading

codecov bot commented Apr 4, 2022 •

edited

Loading

t-nelson left a comment

lijunwangs commented Apr 5, 2022

t-nelson commented Apr 5, 2022

lijunwangs commented Apr 5, 2022 •

edited

Loading

lijunwangs commented Apr 5, 2022

CriesofCarrots commented Apr 12, 2022

lijunwangs commented Apr 12, 2022

t-nelson commented Apr 14, 2022

lijunwangs commented Apr 14, 2022

lijunwangs commented Apr 15, 2022

lijunwangs commented Apr 20, 2022

CriesofCarrots commented Apr 20, 2022

CriesofCarrots left a comment

CriesofCarrots Apr 20, 2022

lijunwangs Apr 21, 2022

CriesofCarrots left a comment

Batch tpu calls in send-transaction-service #24083

Batch tpu calls in send-transaction-service #24083

Conversation

lijunwangs commented Apr 2, 2022 • edited Loading

Problem

Summary of Changes

codecov bot commented Apr 4, 2022 • edited Loading

Codecov Report

t-nelson left a comment

Choose a reason for hiding this comment

lijunwangs commented Apr 5, 2022

t-nelson commented Apr 5, 2022

lijunwangs commented Apr 5, 2022 • edited Loading

lijunwangs commented Apr 5, 2022

CriesofCarrots commented Apr 12, 2022

lijunwangs commented Apr 12, 2022

t-nelson commented Apr 14, 2022

lijunwangs commented Apr 14, 2022

lijunwangs commented Apr 15, 2022

lijunwangs commented Apr 20, 2022

CriesofCarrots commented Apr 20, 2022

CriesofCarrots left a comment

Choose a reason for hiding this comment

CriesofCarrots Apr 20, 2022

Choose a reason for hiding this comment

lijunwangs Apr 21, 2022

Choose a reason for hiding this comment

CriesofCarrots left a comment

Choose a reason for hiding this comment

lijunwangs commented Apr 2, 2022 •

edited

Loading

codecov bot commented Apr 4, 2022 •

edited

Loading

lijunwangs commented Apr 5, 2022 •

edited

Loading